Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

m1heng
Copy link
Contributor

@m1heng m1heng commented Oct 12, 2024

This pull request aligns the convertValue function in the otellogrus package with the otellogr.
Also fix #6235

@m1heng m1heng requested review from dmathieu, pellared and a team as code owners October 12, 2024 08:15
@pellared
Copy link
Member

pellared commented Oct 12, 2024

It would be better to put convertValue code under internal/shared and use //go:generate gotmpl to reuse the code in log bridges (otellogrus, otelzap, otellogr). Do you want to try doing it?

@m1heng m1heng force-pushed the feat/align-type branch 2 times, most recently from 7d3a3be to 6aa37ba Compare October 13, 2024 03:10
@m1heng
Copy link
Contributor Author

m1heng commented Oct 13, 2024

@pellared just did it, also added some nil pointer test.
Also this would leads to a semi-breaking change.
e.g.

  1. in otellogrus, time attr would become nanoseconds and many other unhandled attribute type.
  2. in otellogrus and otelzap unsupported type is marked as unhandled attribute type: (%s) %+v, but now would be unhandled: (%s) %+v.

@m1heng m1heng changed the title Align logrus convertValue to logr Move convert values to internal/shared/logutil Oct 13, 2024
@m1heng m1heng mentioned this pull request Oct 13, 2024
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this would leads to a semi-breaking change.

I think these changes are good 👍

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved
internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved
internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.9%. Comparing base (4e909f0) to head (3f5847a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6237     +/-   ##
=======================================
+ Coverage   66.5%   66.9%   +0.4%     
=======================================
  Files        188     190      +2     
  Lines      12456   12538     +82     
=======================================
+ Hits        8284    8391    +107     
+ Misses      3879    3856     -23     
+ Partials     293     291      -2     
Files with missing lines Coverage Δ
bridges/otellogr/convert.go 100.0% <ø> (+0.9%) ⬆️
bridges/otellogr/logsink.go 98.6% <100.0%> (+0.4%) ⬆️
bridges/otellogrus/convert.go 100.0% <100.0%> (ø)
bridges/otellogrus/hook.go 97.5% <ø> (-0.1%) ⬇️
bridges/otelzap/convert.go 100.0% <100.0%> (ø)
bridges/otelzap/encoder.go 100.0% <ø> (+13.8%) ⬆️

... and 1 file with indirect coverage changes

@pellared
Copy link
Member

Can you also please update the changelog and add an entry for each package where #6235 is fixed.

@m1heng m1heng force-pushed the feat/align-type branch 2 times, most recently from 98baa6a to a1c5708 Compare October 14, 2024 14:01
@m1heng
Copy link
Contributor Author

m1heng commented Oct 14, 2024

done

@pellared pellared changed the title Move convert values to internal/shared/logutil Fix panic for nil attributes and move convert values to internal/shared/logutil Oct 14, 2024
@pellared pellared changed the title Fix panic for nil attributes and move convert values to internal/shared/logutil Fix panic for nil attributes and move convert funcs to internal/shared/logutil Oct 14, 2024
internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved
internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved
@m1heng
Copy link
Contributor Author

m1heng commented Oct 14, 2024

you want func ConvertValue to be private in all package?

@pellared
Copy link
Member

pellared commented Oct 14, 2024

you want func ConvertValue to be private in all package?

Correct. We do not want to extend the exported API surface unless necessary.

@m1heng
Copy link
Contributor Author

m1heng commented Oct 14, 2024

just changed to private

@pellared pellared added this to the v1.32.0 milestone Oct 15, 2024
@pellared pellared merged commit 32c279a into open-telemetry:main Oct 15, 2024
24 checks passed
@pellared
Copy link
Member

@m1heng, thank you very much for your contribution 🏅

pellared added a commit that referenced this pull request Oct 16, 2024
pellared added a commit that referenced this pull request Nov 8, 2024
### Added

- Add the `WithSource` option to the
`go.opentelemetry.io/contrib/bridges/otelslog` log bridge to set the
`code.*` attributes in the log record that includes the source location
where the record was emitted. (#6253)
- Add `ContextWithStartTime` and `StartTimeFromContext` to
`go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`, which
allows setting the start time using go context. (#6137)
- Set the `code.*` attributes in
`go.opentelemetry.io/contrib/bridges/otelzap` if the `zap.Logger` was
created with the `AddCaller` or `AddStacktrace` option. (#6268)
- Add a `LogProcessor` to
`go.opentelemetry.io/contrib/processors/baggagecopy` to copy baggage
members to log records. (#6277)
  - Use `baggagecopy.NewLogProcessor` when configuring a Log Provider.
- `NewLogProcessor` accepts a `Filter` function type that selects which
baggage members are added to the log record.

### Changed 

- Transform raw (`slog.KindAny`) attribute values to matching
`log.Value` types.
For example, `[]string{"foo", "bar"}` attribute value is now transformed
to `log.SliceValue(log.StringValue("foo"), log.StringValue("bar"))`
instead of `log.String("[foo bar"])`. (#6254)
- Upgrade `go.opentelemetry.io/otel/semconv/v1.17.0` to
`go.opentelemetry.io/otel/semconv/v1.21.0` in
`go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo`.
(#6272)
- Resource doesn't merge with defaults if a valid resource is configured
in `go.opentelemetry.io/contrib/config`. (#6289)

### Fixed

- Transform nil attribute values to `log.Value` zero value instead of
panicking in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237)
- Transform nil attribute values to `log.Value` zero value instead of
panicking in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237)
- Transform nil attribute values to `log.Value` zero value instead of
`log.StringValue("<nil>")` in
`go.opentelemetry.io/contrib/bridges/otelslog`. (#6246)
- Fix `NewClientHandler` so that `rpc.client.request.*` metrics measure
requests instead of responses and `rpc.client.responses.*` metrics
measure responses instead of requests in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`.
(#6250)
- Fix issue in `go.opentelemetry.io/contrib/config` causing
`otelprom.WithResourceAsConstantLabels` configuration to not be
respected. (#6260)
- `otel.Handle` is no longer called on a successful shutdown of the
Prometheus exporter in `go.opentelemetry.io/contrib/config`. (#6299)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nil pointer field cause panic
4 participants